fix: minor improvements to event attendance#310
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughActivistRegistry no longer accepts storage in its constructor; storage is provided to loadFromStorage(storage). loadFromStorage rebuilds the in-memory activists list and id/name indexes. mergeActivists now merges in-memory and then rebuilds the name index. removeActivistsByIds now uses an explicit loop to collect remaining activists, updates indexes, and writes deletions through storage when configured. Changes
Sequence Diagram(s)sequenceDiagram
participant Hook as rgba(63,81,181,0.5) Hook (useActivistRegistry)
participant Registry as rgba(0,150,136,0.5) ActivistRegistry
participant Storage as rgba(255,87,34,0.5) ActivistStorage
Hook->>Registry: new ActivistRegistry()
Hook->>Registry: loadFromStorage(activistStorage)
Registry->>Storage: readAll()
Storage-->>Registry: activists[]
Registry->>Registry: rebuild in-memory list, id & name indexes
Hook->>Registry: mergeActivists(activistsToUpdate)
Registry->>Registry: merge in-memory, rebuild name index
Registry->>Storage: write updates/deletions (if configured)
Storage-->>Registry: ack
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
alexsapps
force-pushed
the
alex/event-fixups
branch
from
4ef8b47 to
67a715d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend-v2/src/app/event/activist-registry.ts (1)
18-21:⚠️ Potential issue | 🟡 MinorConstructor JSDoc is stale after the signature change.
The comment still documents a
storageconstructor parameter, butconstructor()no longer accepts one. Please update this block to avoid incorrect usage guidance.📝 Suggested doc fix
/** * In-memory activist registry with write-through storage to IndexedDB. * * Reads are synchronous (from memory) for fast autocomplete/filtering. * Writes are async and automatically persist to IndexedDB when storage is configured. - * - * `@param` storage - Optional IndexedDB storage for persistence. - * When provided, enables automatic write-through caching. - * When omitted, registry operates in memory-only mode. */ export class ActivistRegistry {Also applies to: 28-28
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend-v2/src/app/event/activist-registry.ts` around lines 18 - 21, Update the stale constructor JSDoc for the ActivistRegistry class: remove or replace the `@param` storage description since constructor() no longer accepts a storage parameter, and instead document the current initialization behavior (e.g., in-memory-only mode or how persistence is configured elsewhere). Edit the JSDoc block above the constructor() in activist-registry.ts to reflect the new signature and initialization semantics so callers aren’t misled by a nonexistent `storage` parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend-v2/src/app/event/activist-registry.ts`:
- Line 62: mergeActivists currently only replaces existing entries by id, but
the contract requires replacement by id OR by name; update mergeActivists to
treat an incoming activist as a duplicate if either id or name matches an
existing record (use a map keyed by id and a secondary map keyed by normalized
name or check both on lookup), and when a match is found overwrite the existing
entry rather than appending; ensure you update both lookup maps (id and name)
and the resulting activists array so stale same-name/different-id records are
removed/replaced (refer to mergeActivists and the activists accumulation logic
in this file).
---
Outside diff comments:
In `@frontend-v2/src/app/event/activist-registry.ts`:
- Around line 18-21: Update the stale constructor JSDoc for the ActivistRegistry
class: remove or replace the `@param` storage description since constructor() no
longer accepts a storage parameter, and instead document the current
initialization behavior (e.g., in-memory-only mode or how persistence is
configured elsewhere). Edit the JSDoc block above the constructor() in
activist-registry.ts to reflect the new signature and initialization semantics
so callers aren’t misled by a nonexistent `storage` parameter.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
frontend-v2/src/app/event/activist-registry.tsfrontend-v2/src/app/event/activist-storage.tsfrontend-v2/src/app/event/useActivistRegistry.ts
|
|
||
| /** | ||
| * Merges new activists with existing data, replacing duplicates by id. | ||
| * Merges new activists with existing data, replacing duplicates by id and name. |
There was a problem hiding this comment.
mergeActivists does not currently replace duplicates by name.
The method contract says replacement happens by id and name, but the implementation only chooses replacement candidates by id. A same-name/different-id incoming record is appended instead of replacing, leaving stale entries and inconsistent semantics.
🔧 Suggested fix for name-based replacement path
async mergeActivists(newActivists: ActivistRecord[]): Promise<void> {
if (newActivists.length === 0) {
return
}
const indexById = new Map(this.activists.map((a, i) => [a.id, i]))
for (const activist of newActivists) {
- const existingIndex = indexById.get(activist.id) ?? -1
+ const existingByName = this.activistsByName.get(activist.name)
+ const existingIndexById = indexById.get(activist.id) ?? -1
+ const existingIndexByName =
+ existingByName ? (indexById.get(existingByName.id) ?? -1) : -1
+ const existingIndex =
+ existingIndexById >= 0 ? existingIndexById : existingIndexByName
if (existingIndex >= 0) {
// Update existing activist (handles renames properly)
const oldActivist = this.activists[existingIndex]
this.activists[existingIndex] = activist
// Remove old name from index if name changed
if (oldActivist.name !== activist.name) {
this.activistsByName.delete(oldActivist.name)
}
+ if (oldActivist.id !== activist.id) {
+ this.activistsById.delete(oldActivist.id)
+ indexById.delete(oldActivist.id)
+ }
+ indexById.set(activist.id, existingIndex)
} else {
// Add new activist
this.activists.push(activist)
indexById.set(activist.id, this.activists.length - 1)
}
// Update indexes
this.activistsByName.set(activist.name, activist)
this.activistsById.set(activist.id, activist)
}Also applies to: 65-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend-v2/src/app/event/activist-registry.ts` at line 62, mergeActivists
currently only replaces existing entries by id, but the contract requires
replacement by id OR by name; update mergeActivists to treat an incoming
activist as a duplicate if either id or name matches an existing record (use a
map keyed by id and a secondary map keyed by normalized name or check both on
lookup), and when a match is found overwrite the existing entry rather than
appending; ensure you update both lookup maps (id and name) and the resulting
activists array so stale same-name/different-id records are removed/replaced
(refer to mergeActivists and the activists accumulation logic in this file).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend-v2/src/app/event/activist-registry.ts (1)
12-32:⚠️ Potential issue | 🟡 MinorStale JSDoc:
@param storageno longer applies to constructor.The
@param storagedocumentation describes a constructor parameter that was removed. Storage is now passed toloadFromStorage()instead. Update the docs to reflect the new API.📝 Suggested fix
/** * In-memory activist registry with write-through storage to IndexedDB. * * Reads are synchronous (from memory) for fast autocomplete/filtering. * Writes are async and automatically persist to IndexedDB when storage is configured. - * - * `@param` storage - Optional IndexedDB storage for persistence. - * When provided, enables automatic write-through caching. - * When omitted, registry operates in memory-only mode. + * + * Call `loadFromStorage()` after construction to enable IndexedDB persistence. + * When storage is not configured, the registry operates in memory-only mode. */ export class ActivistRegistry {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend-v2/src/app/event/activist-registry.ts` around lines 12 - 32, Update the class JSDoc for ActivistRegistry to remove the outdated "@param storage" line and replace it with a brief note that persistence is controlled via loadFromStorage(storage) rather than a constructor parameter; mention that the registry defaults to in-memory mode and that providing storage to loadFromStorage enables write-through persistence to IndexedDB, and reference the loadFromStorage method and the optional storage property in the description so readers can find the new API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@frontend-v2/src/app/event/activist-registry.ts`:
- Around line 12-32: Update the class JSDoc for ActivistRegistry to remove the
outdated "@param storage" line and replace it with a brief note that persistence
is controlled via loadFromStorage(storage) rather than a constructor parameter;
mention that the registry defaults to in-memory mode and that providing storage
to loadFromStorage enables write-through persistence to IndexedDB, and reference
the loadFromStorage method and the optional storage property in the description
so readers can find the new API.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes from #297
Summary by CodeRabbit
Documentation
Refactor
Bug Fixes